-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refetch even when the query has not changed #13759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just left one minor suggestion about function names.
@@ -465,8 +461,8 @@ function discoverController( | |||
if ($state.query.language && $state.query.language !== query.language) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I called these methods updateQuery
since they only update state. Now that they also fetch results, maybe we should call them something like fetchWithQuery
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 9c27c33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That name sounds good to me! Did you forget to update all the usages though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How embarrassing. ef09fa0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to fix the issue, but leads to updateQueryAndFetch
being called twice when the query has changed: once for the "submit" and once triggered by the $watch('state.query', ...)
. Is there maybe some way to get rid of that $watch
?
For background, the watch is there in order to ensure old queries get migrated. Queries on saved searches get migrated at load time, but a query might also be stored in appstate in the url of a bookmark or link. It was the only thing I could think of to ensure the app is always working with an updated query object. |
@weltenwort Invoking the function multiple times doesn't result in multiple HTTP requests, because of how the courier works. That being said, I agree there is a bit of code smell there, but the only thing I can think of to prevent this would be to manually check if the query object has changed and return early if it has (since it will be called inside the watch). Thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking some more about it I can't think of an elegant fix either. Relying on the deduplication feature of courier is probably the way to go here.
* Refetch even when the query has not changed * Change function name to better represent what it does * Rename all occurrences
* Refetch even when the query has not changed * Change function name to better represent what it does * Rename all occurrences
Fixes #13743.
This PR makes it so any time a query is submitted (in Discover/Visualize/Dashboard), a fetch happens, even if the query itself doesn't change.